Add search backpressure cancellation at the coordinator level #5605

PritLadani · 2022-12-19T18:22:45Z

Description

Cancellation of in-flight SearchTasks based on resource consumption.

This feature aims to identify and cancel resource intensive SearchTasks if they have breached certain
thresholds. This will help in terminating problematic queries which can put nodes in duress and degrade the
cluster performance.

For resource tracking of SearchTasks, we will use the existing thresholds used for SearchShardTask but with different thresholds.
Introduced new dynamic settings for defining thresholds for SearchTask cancellation.
Added search_task stats in the response body of _nodes/stats/search_backpressure.

Issues Resolved

#5173

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: PritLadani [email protected]

Signed-off-by: PritLadani <[email protected]>

…com:PritLadani/OpenSearch into searchbackpressure/search-task-cancellation

github-actions · 2022-12-19T18:51:37Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/8264/
CommitID: 1402ced
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-12-19T18:59:41Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/8265/
CommitID: 253fc61
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

Signed-off-by: PritLadani <[email protected]>

github-actions · 2022-12-20T07:50:24Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/8276/
CommitID: 5817770
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

…-task-cancellation

github-actions · 2022-12-20T08:29:44Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/8277/
CommitID: dee7efb
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

reta · 2023-02-03T14:16:54Z

server/src/main/java/org/opensearch/search/backpressure/SearchBackpressureService.java

+    ) {
+        List<TaskResourceUsageTracker> trackers = new ArrayList<>();
+        trackers.add(new CpuUsageTracker(cpuThresholdSupplier));
+        if (HEAP_SIZE_BYTES > 0) {


You could have this check on HeapUsageTracker as a static method, fe isHeapTrackingSupported. that would clearly communicate what is the purpose:

if (HeapUsageTracker.isHeapTrackingSupported()) { ... }

If you want to push it further and seal the implementation, introduce the factory method to HeapUsageTracker that return Optional<HeapUsageTracker>, for consumers there won't be any doubts if heap tracking is supported or not:

class HeapUsageTracker { static Optional<HeapUsageTracker> create() { } }

Makes sense. Updated in the next commit.

Signed-off-by: PritLadani <[email protected]>

…com:PritLadani/OpenSearch into searchbackpressure/search-task-cancellation

github-actions · 2023-02-03T15:30:41Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.http.SearchRestCancellationIT.testAutomaticCancellationMultiSearchDuringFetchPhase
      1 org.opensearch.http.SearchRestCancellationIT.testAutomaticCancellationDuringFetchPhase

URL: https://build.ci.opensearch.org/job/gradle-check/10589/
CommitID: e8df261
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

reta · 2023-02-03T15:43:54Z

server/src/main/java/org/opensearch/search/backpressure/settings/SearchShardTaskSettings.java

@@ -52,7 +188,96 @@ public long getTotalHeapBytesThreshold() {
        return (long) (HEAP_SIZE_BYTES * getTotalHeapPercentThreshold());


The HEAP_SIZE_BYTES leaks here as well, so when it is 0, both thresholds are going to be incorrect: SearchBackpressureService::isHeapUsageDominatedBySearch will always return true. I think we have to move this getTotalHeapBytesThreshold to HeapTracker as well, so the decision regarding heap tracking is made in once place only.

How about we change the method isHeapUsageDominatedBySearch like this?

boolean isHeapUsageDominatedBySearch(List<CancellableTask> cancellableTasks, double heapPercentThreshold) { long usage = cancellableTasks.stream().mapToLong(task -> task.getTotalResourceStats().getMemoryInBytes()).sum(); long threshold = (long) heapPercentThreshold * getHeapSizeBytes(); if (isHeapTrackingSupported() && usage < threshold) { logger.debug("heap usage not dominated by search requests [{}/{}]", usage, threshold); return false; } return true; }

👍 This is better, thank you

Well, I meant to keep it in SearchBackpressureService only. Hope that's fine?
Like, I don't see a problem in having a public getter for HEAP_SIZE_BYTES. Thoughts?

The HEAP_SIZE_BYTES should be sealed: you see, your settings would have incorrect values because the knowledge that HEAP_SIZE_BYTES could be 0 is implicit. This is not good, by sealing it and only having method to access it you free up the users of thinking that HEAP_SIZE_BYTES could be incorrect.

I did not get it. Should we have a public getter or not?

public getter for getTotalHeapPercentThreshold() - yes, public static long getTotalHeapBytesThreshold(double threshold ) goes to HeapTracker, HEAP_SIZE_BYTES becomes private

Makes sense. Updated the code.

reta · 2023-02-03T15:53:23Z

server/src/main/java/org/opensearch/search/backpressure/trackers/HeapUsageTracker.java

-        Setting.Property.NodeScope
-    );
-
+    public static final long HEAP_SIZE_BYTES = JvmStats.jvmStats().getMem().getHeapMax().getBytes();


The HEAP_SIZE_BYTES has to go back to private please

We need the access of HEAP_SIZE_BYTES in Search[Shard]TaskSettings for getHeapBytesThreshold since that should not be moved to HeapUsageTracker. I will access the value using a public method HeapUsageTracker.getHeapSizeBytes. Hope that's fine.

You could always calculate the threshold based on getHeapThreshold (this is the only setting you need), and HeapUsageTracker knows how calculate the threshold in bytes (based on HEAP_SIZE_BYTES)

Signed-off-by: PritLadani <[email protected]>

reta · 2023-02-03T18:28:41Z

I think it is looking good @PritLadani , thank you. Could you please create a complement documentation issue here [1] so the new settings will be properly documented, thank you.

[1] https://github.com/opensearch-project/documentation-website/issues

reta · 2023-02-03T18:30:31Z

@Bukhtawar mind taking a look please when you have a chance? thank you.

Bukhtawar · 2023-02-03T18:37:10Z

server/src/main/java/org/opensearch/search/backpressure/SearchBackpressureService.java

+        // Check if increase in heap usage is due to SearchTasks
+        if (HeapUsageTracker.isHeapUsageDominatedBySearch(
+            searchTasks,
+            getSettings().getSearchTaskSettings().getTotalHeapPercentThreshold()
+        )) {
+            cancellableTasks.addAll(searchTasks);
+        }
+
+        // Check if increase in heap usage is due to SearchShardTasks
+        if (HeapUsageTracker.isHeapUsageDominatedBySearch(
+            searchShardTasks,
+            getSettings().getSearchShardTaskSettings().getTotalHeapPercentThreshold()
+        )) {
+            cancellableTasks.addAll(searchShardTasks);
+        }
+


redundant code

I have discussed this with @ketanv3 and decided that we will keep this for now. We can decide on removing this check in the later phase.

Also why is domination heap based. Can it be CPU dominated as well?

It can be and @ketanv3 is doing some rounds of testing with this. We will address this in the next PR.

Bukhtawar · 2023-02-03T18:39:23Z

server/src/main/java/org/opensearch/search/backpressure/stats/SearchBackpressureStats.java

+        searchShardTaskStats = new SearchShardTaskStats(in);
+        mode = SearchBackpressureMode.fromName(in.readString());
+        if (in.getVersion().onOrAfter(Version.V_3_0_0)) {
+            searchTaskStats = in.readOptionalWriteable(SearchTaskStats::new);
+        } else {
+            searchTaskStats = null;
+        }
    }


Set version to V_2_6 and then backport

@Bukhtawar that will fail all main builds :(, including this pull request, we probably should merge -> backport -> and fix main

I wish we had a better way to do this, but I agree that @reta's suggestion is the least disruptive (and avoids the bad look of merging a PR with failing tests).

Wouldn't the BWC tests fail between the backport and the second main fix. The second problem is the automatic back port starts to fail since it would need a change to point to 2_6. Seems just too much overhead on backports.
Instead if we can auto-trigger a backport PR and keep it handy before we merge to main and merge backport shortly after, it minimises the chances of build failures and simplifies the process.

@reta @andrross Thoughts?

Thanks @Bukhtawar, yes, once 2.x backport is merged, the main would start to fail but:

the time window between main being broken and 2.x being merged is unpredictable, could take hours blocking every other merge to main

the pull request to main could be sent right with backport to 2.x - yes, it will fail, but it is just a matter of restarting the Gradle check to get it merged (after 2.x merge)

The process is far from ideal, but the disruption time would be minimal in this case, I think. Also, we should never merge the pull requests with failing check - this is not possible with the main change going first.

andrross · 2023-02-03T18:42:44Z

CHANGELOG.md

@@ -9,6 +9,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Add getter for path field in NestedQueryBuilder ([#4636](https://github.com/opensearch-project/OpenSearch/pull/4636))
 - Allow mmap to use new JDK-19 preview APIs in Apache Lucene 9.4+ ([#5151](https://github.com/opensearch-project/OpenSearch/pull/5151))
 - Add support for ppc64le architecture ([#5459](https://github.com/opensearch-project/OpenSearch/pull/5459))
+- Cancellation of in-flight SearchTasks based on resource consumption ([#5606](https://github.com/opensearch-project/OpenSearch/pull/5605))


Will this go into the next 2.x release? If so, please move this to the unreleased 2.x section and make sure the appropriate labels get added to the PR.

Also, it's probably better to link to the issue (#5173), and can you rewrite this in the imperative mood? Maybe something like "Add backpressure cancellation of search tasks at the coordinator level"? But please feel free to rewrite that.

Well yes, this should go in 2.x release, will move it to unreleased 2.x section and update labels.
Sure, will rewrite it as "Add search backpressure cancellation at the coordinator level".
About linking the issue, I have already mentioned the issue id in the PR description. What else should be done here?

Edit: I am not able to add the labels yet. Is it possible for you to add the labels? or @Bukhtawar can you help me add the labels to the PR?

I've added the labels. I just mean put the link to the issue instead of the PR in the changelog. It has more relevant information for a user that wants to learn more about this change.

Thanks for updating the labels.
Agree on relevancy but I do not see any entry with the issue link. Am I missing something or it's fine to add issue link?

github-actions · 2023-02-03T18:46:31Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/10601/
CommitID: 881bdee
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

Signed-off-by: PritLadani <[email protected]>

github-actions · 2023-02-03T19:36:50Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/10605/
CommitID: 5da5817

github-actions · 2023-02-05T09:37:10Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/10653/
CommitID: bf19bf7

opensearch-trigger-bot · 2023-02-05T13:33:34Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-5605-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 74912d252e803c67b18ac74de508d1e87d1bc3c4
# Push it to GitHub
git push --set-upstream origin backport/backport-5605-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-5605-to-2.x.

…arch-project#5605) * Cancellation of in-flight search requests at coordinator level Signed-off-by: PritLadani <[email protected]> (cherry picked from commit 74912d2)

…arch-project#5605) * Cancellation of in-flight search requests at coordinator level Signed-off-by: PritLadani <[email protected]> (cherry picked from commit 74912d2) Signed-off-by: PritLadani <[email protected]>

…tor level (#5605) (#6194) * Add search backpressure cancellation at the coordinator level (#5605) * Cancellation of in-flight search requests at coordinator level Signed-off-by: PritLadani <[email protected]>

Cancellation of in-flight search requests at coordinator level

1402ced

Signed-off-by: PritLadani <[email protected]>

PritLadani requested review from reta, anasalkouz, andrross, Bukhtawar, CEHENKLE, dblock, setiah, kartg, kotwanikunal, mch2, nknize, owaiskazi19, adnapibar, Rishikesh1159, ryanbogan, saratvemulapalli, shwetathareja, dreamer-89, tlfeng, VachaShah and xuezhou25 as code owners December 19, 2022 18:22

PritLadani added 2 commits December 19, 2022 23:56

Cancellation of in-flight search requests at coordinator level

9ccdae5

Signed-off-by: PritLadani <[email protected]>

Merge branch 'searchbackpressure/search-task-cancellation' of github.…

253fc61

…com:PritLadani/OpenSearch into searchbackpressure/search-task-cancellation

Fixing test failures

5817770

Signed-off-by: PritLadani <[email protected]>

Merge branch 'opensearch-project:main' into searchbackpressure/search…

dee7efb

…-task-cancellation

reta reviewed Feb 3, 2023

View reviewed changes

PritLadani added 2 commits February 3, 2023 20:30

Adding method to check if heap usage tracking is enabled

874cd97

Signed-off-by: PritLadani <[email protected]>

Merge branch 'searchbackpressure/search-task-cancellation' of github.…

e8df261

…com:PritLadani/OpenSearch into searchbackpressure/search-task-cancellation

reta reviewed Feb 3, 2023

View reviewed changes

Isolating HEAP_SIZE_BYTES value

881bdee

Signed-off-by: PritLadani <[email protected]>

Bukhtawar reviewed Feb 3, 2023

View reviewed changes

andrross reviewed Feb 3, 2023

View reviewed changes

PritLadani changed the title ~~Cancellation of in-flight SearchTasks based on resource consumption~~ Add search backpressure cancellation at the coordinator level Feb 3, 2023

Moving changelog to unreleased 2.x section

5da5817

Signed-off-by: PritLadani <[email protected]>

andrross added backport 2.x Backport to 2.x branch v2.6.0 'Issues and PRs related to version v2.6.0' labels Feb 3, 2023

PritLadani mentioned this pull request Feb 3, 2023

[DOC] Search backpressure cluster settings documentation opensearch-project/documentation-website#2629

Closed

4 tasks

Merge branch 'main' into searchbackpressure/search-task-cancellation

bf19bf7

Bukhtawar approved these changes Feb 5, 2023

View reviewed changes

Bukhtawar merged commit 74912d2 into opensearch-project:main Feb 5, 2023

PritLadani mentioned this pull request Mar 2, 2023

[DOC] Search backpressure SearchTask cancellation documentation opensearch-project/documentation-website#3172

Open

4 tasks

PritLadani mentioned this pull request Jul 5, 2023

Search Query Runtime Cost Calculation #5174

Open

PritLadani mentioned this pull request Jul 14, 2023

Add search backpressure cancellation at the coordinator level #5173

Closed

		@@ -52,7 +188,96 @@ public long getTotalHeapBytesThreshold() {
		return (long) (HEAP_SIZE_BYTES * getTotalHeapPercentThreshold());

Add search backpressure cancellation at the coordinator level #5605

Add search backpressure cancellation at the coordinator level #5605

Conversation

PritLadani commented Dec 19, 2022 • edited Loading

Description

Issues Resolved

Check List

github-actions bot commented Dec 19, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Dec 19, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Dec 20, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Dec 20, 2022

Gradle Check (Jenkins) Run Completed with:

reta Feb 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 3, 2023

Gradle Check (Jenkins) Run Completed with:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reta Feb 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reta Feb 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reta Feb 3, 2023 • edited Loading

Choose a reason for hiding this comment

reta commented Feb 3, 2023

reta commented Feb 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bukhtawar Feb 4, 2023 • edited Loading

Choose a reason for hiding this comment

reta Feb 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PritLadani Feb 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PritLadani Feb 3, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Feb 3, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Feb 3, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Feb 5, 2023

Gradle Check (Jenkins) Run Completed with:

opensearch-trigger-bot bot commented Feb 5, 2023

PritLadani commented Dec 19, 2022 •

edited

Loading

reta Feb 3, 2023 •

edited

Loading

reta Feb 3, 2023 •

edited

Loading

reta Feb 3, 2023 •

edited

Loading

reta Feb 3, 2023 •

edited

Loading

Bukhtawar Feb 4, 2023 •

edited

Loading

reta Feb 4, 2023 •

edited

Loading

PritLadani Feb 3, 2023 •

edited

Loading

PritLadani Feb 3, 2023 •

edited

Loading